diagnostics_channel: add opt-in subscriber suppression via bypassBy and bypassed() by DivyanshuX9 · Pull Request #63651 · nodejs/node

DivyanshuX9 · 2026-05-29T21:31:19Z

Summary

Adds opt-in subscriber suppression to diagnostics_channel so APM
agents can prevent recursive instrumentation when their own internal
code calls into instrumented libraries.

What Changed

`lib/diagnostics_channel.js`

Added suppressed(key, fn, thisArg, ...args) - runs fn with
the given key active in the current async context
subscribe() and bindStore() accept { subscriberId } option
Separate lazy storage for bypassed subscribers/stores - normal
path has zero overhead, ALS is only hit when bypass subscribers exist
Lazy AsyncLocalStorage initialization via getSuppressionsStorage()
- required because diagnostics_channel loads during bootstrap before
  async_hooks is available
Unified validateBypassKey() helper for all key validation

`test/parallel/test-diagnostics-channel-suppression.js`

10 test scenarios covering sync, async, Promise, timer,
store, nested, and type validation cases

Why

APM agents currently implement suppression themselves using a
custom ALS noop marker - every vendor reimplements the same
pattern differently. Moving it into core:

Eliminates per-handler overhead on the hot path
Gives vendors a standard API
Correctly propagates across Promise/timer boundaries via ALS

Backward Compatibility

Fully opt-in : existing code unaffected
Normal subscribers pay zero extra cost
ALS only initialized and queried when bypass subscribers exist

Naming

As per @bengl final name is "Bypass"

Addresses:#63623

Defer non-critical warnings to the next event loop iteration when can_call_into_js() returns false. This prevents crashes when V8 emits warnings during REPL preview evaluation or other contexts where JavaScript execution is temporarily forbidden. When a warning is emitted inside DisallowJavascriptExecutionScope, ProcessEmitWarningGeneric cannot be called immediately. Instead, use env->SetImmediate() to queue the warning emission for after the scope exits. This preserves full warning formatting, deprecation codes, and --redirect-warnings routing. Signed-off-by: Divyanshu Sharma <Divyanshu88999@gmail.com>

Refs: nodejs#63623 Refs: nodejs#63651 Signed-off-by: Divyanshu Sharma <divyanshu88999@gmail.com>

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

DivyanshuX9 · 2026-05-29T22:19:35Z

@BridgeAR @bengl @Qard , I am opening this as a implementation
of #63623. Test file is included. Happy to adjust the API shape
or implementation based on your feedback before CI runs.

Refs: nodejs#63623 Refs: nodejs#63651 Signed-off-by: Divyanshu Sharma <divyanshu88999@gmail.com>

codecov · 2026-05-30T00:06:44Z

Codecov Report

❌ Patch coverage is 97.43590% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.13%. Comparing base (58795e9) to head (a86df32).
⚠️ Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/diagnostics_channel.js	97.43%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #63651      +/-   ##
==========================================
- Coverage   90.16%   90.13%   -0.03%     
==========================================
  Files         744      744              
  Lines      242518   242667     +149     
  Branches    45692    45723      +31     
==========================================
+ Hits       218663   218729      +66     
- Misses      15349    15443      +94     
+ Partials     8506     8495      -11

Files with missing lines	Coverage Δ
lib/diagnostics_channel.js	`97.28% <97.43%> (+<0.01%)`	⬆️

... and 33 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Qard

Tests are missing a lot of necessary common.mustCall(fn) wrappers.

Also, it seems like this was just vibe-coded without reviewing the output before submitting the PR. Please ensure it is in a good state and that the test suite and lint passes before submitting.

Signed-off-by: Divyanshu Sharma <divyanshu88999@gmail.com>

…update tests to use common.mustCall

DivyanshuX9 · 2026-05-30T09:58:52Z

@Qard , thanks for the detailed review. So far the checklist of what i have fixed in the latest push:

[] Removed the impossible ALS try/catch fallback
[] Removed node_errors.cc changes as you suggested
[] Replaced all IIFE wrappers with plain blocks in tests
[] Added common.mustCall() / common.mustNotCall() to all handlers
[] Ran lint and tests locally , both passing

Ready for re-review and any changes if you want, when you have time please review it.

Signed-off-by: Divyanshu Sharma <divyanshu88999@gmail.com>

Qard · 2026-05-30T13:12:53Z

The node_errors.cc change still seems to be present.

…ove SetImmediate deferral) Signed-off-by: Divyanshu Sharma <divyanshu88999@gmail.com>

Signed-off-by: Divyanshu Sharma <divyanshu88999@gmail.com>

DivyanshuX9 · 2026-07-01T21:16:59Z

@BridgeAR @bengl @rochdev
I opened the docs PR: #64250

Covers bypass(), bypassId on subscribe/bindStore/tracingChannel,the APM recursion use case, and multi-vendor coordination.
Both MJS and CJS examples throughout.

Tell if i'm missing somethin' or I need to add anything else

Flarna

Docs are missing

DivyanshuX9 · 2026-07-02T19:53:06Z

@Flarna docs are in PR: #64250
since it is going to be a continuation of this PR , after all the changes and reviews i'll update them.

Flarna · 2026-07-03T06:28:25Z

I think implementation + tests + docs of a new public API should come in one PR.

DivyanshuX9 · 2026-07-03T07:58:29Z

@Flarna Well once BridgeAR also mentioned about doc

We definitely also need documentation for the feature.

at that time was not sure how far reviews will go ,also @rochdev wanted some examples rightaway, So made a seperate PR for that to keep this one clean , yes if i get a green signal to land this one will will update and merge that pr doc in this one.

Signed-off-by: Divyanshu Sharma <divyanshu88999@gmail.com>

DivyanshuX9 · 2026-07-06T15:42:22Z

@Flarna i have moved all the docs here

Signed-off-by: Divyanshu Sharma <divyanshu88999@gmail.com>

DivyanshuX9 · 2026-07-08T13:56:47Z

@Flarna , lowkey I have addressed all feedback in latest push:

Added Stability: 1.1 - Active development to bypass()
Removed bypass example from Overview (not a typical usage pattern)
Rewrote key parameter description to clarify no single match required
Rewrote description paragraph to focus on behavior, not internals
Fixed channel name to http.client.request.start throughout
Replaced repeated subscribe/bindStore bypass examples with link to bypass() docs
Removed all the unrelated REPLACEME changes entry from tracingChannel.subscribe

Thank you for the thorough review.

DivyanshuX9 · 2026-07-08T14:00:12Z

Stability: 1.1 - Active development

also @Flarna at 251 line is this correct or should i just remove REPLACE ME with the stability:1.1 right away

Flarna · 2026-07-08T15:09:11Z

No, the REPLACEME is changed by the release tooling when a release is made, see https://github.com/nodejs/node/blob/main/doc/contributing/api-documentation.md#writing-documentation

The stability index is just documentation and independent of that.

Signed-off-by: Divyanshu Sharma <divyanshu88999@gmail.com>

DivyanshuX9 · 2026-07-15T07:51:15Z

@BridgeAR following up on your performance concern, ran a microbenchmark on the normal-subscriber hot path (1,000,000 iterations, no bypass() active):

	main	branch	delta
normal-only (no bypass sub)	5.77ms	7.97ms	+2.2ms (+38%)
normal + bypass sub (bypass idle)	14.9ms	19.6ms	+4.7ms (+32%)

Test 1 is the pure normal path with no bypass subscriber registered at all, the overhead is just the _bypassSubscribers !== null null-check plus the extra fields on the channel object, comes out to around 2.2ns per call.
Test 2 is when an APM tool has opted in but bypass() isn't currently active, getBypassStorage().getStore() runs on every publish anyway, that's where the bigger cost comes from (~4.7ns/call over baseline).
Before I touch anything wanted to ask, is the 2.2ns on the pure normal path within acceptable range for this feature, or do you want me to look at avoiding the hidden-class shape change? For test 2 I could try caching the getStore() result per microtask turn but that adds complexity, not sure if its worth it.
Happy to dig into either if you think it matters.

Signed-off-by: DivyanshuX9 <divyanshu88999@gmail.com>

Signed-off-by: Divyanshu Sharma <divyanshu88999@gmail.com>

Three bad-merge fragments were committed without conflict markers: - unsubscribe: spurious decRef/maybeMarkInactive before return false - bindStore: stale bindStore(store, transform) fragment injected inside the else block of bindStore(store, transform, options), with missing closing braces for the else block and the method - unbindStore: spurious delete/decRef/maybeMarkInactive before return false All three were remnants of the pre-bypass single-path implementation that were not removed during the bypass-feature merge. Signed-off-by: Divyanshu Sharma <divyanshu88999@gmail.com>

…s entries Signed-off-by: Divyanshu Sharma <divyanshu88999@gmail.com>

The subscriber buffer (dc_binding.subscribers) is replaced with a new larger TypedArray when native channel storage grows past its initial capacity. The old code destructured it once at module load: const { subscribers: subscriberCounts } = dc_binding; After growth, dc_binding.subscribers points to the new array but subscriberCounts still holds a reference to the old one. Any subscribe/unsubscribe/bindStore/unbindStore call after growth would increment/decrement the stale array, leaving the live array at zero. Channel::HasSubscribers() reads from the live array and would return false even with active subscribers, breaking the C++ test NativeChannelsGrowSubscriberStorage. Fix: remove the cached destructuring and access dc_binding.subscribers directly at every call site. The comment above the binding already stated this requirement; the code just did not follow it. Signed-off-by: Divyanshu Sharma <divyanshu88999@gmail.com>

ActiveChannel.publish() accessed this._subscribers with optional chaining (subscribers?.length || 0). This is unnecessary because: - _subscribers is always initialized to [] by markActive() before any ActiveChannel method can be called - maybeMarkInactive() sets _subscribers back to undefined only after switching the prototype back to Channel.prototype, so ActiveChannel methods are never reachable with a nullish _subscribers Use subscribers.length directly. Signed-off-by: Divyanshu Sharma <divyanshu88999@gmail.com>

DivyanshuX9 and others added 2 commits May 24, 2026 21:33

Merge branch 'nodejs:main' into fix/issue-63473-repl-asm-warning

7b20b8a

Copilot AI review requested due to automatic review settings May 29, 2026 21:31

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. diagnostics_channel Issues and PRs related to diagnostics channel needs-ci PRs that need a full CI run. labels May 29, 2026

DivyanshuX9 mentioned this pull request May 29, 2026

diagnostics_channel: opt-in subscriber suppression #63623

Open

DivyanshuX9 added a commit to DivyanshuX9/node that referenced this pull request May 29, 2026

lib: fix diagnostics_channel lint issues

8b122c2

Refs: nodejs#63623 Refs: nodejs#63651 Signed-off-by: Divyanshu Sharma <divyanshu88999@gmail.com>

DivyanshuX9 force-pushed the diag/suppression-als branch from 5b4110a to 8b122c2 Compare May 29, 2026 21:49

Copilot AI reviewed May 29, 2026

DivyanshuX9 added 2 commits May 30, 2026 04:07

lib: diagnostics_channel use AsyncLocalStorage for suppression context

a786501

Refs: nodejs#63623 Refs: nodejs#63651 Signed-off-by: Divyanshu Sharma <divyanshu88999@gmail.com>

test: add diagnostics_channel suppression coverage

e4aea85

Refs: nodejs#63623 Refs: nodejs#63651 Signed-off-by: Divyanshu Sharma <divyanshu88999@gmail.com>

DivyanshuX9 force-pushed the diag/suppression-als branch from 8b122c2 to e4aea85 Compare May 29, 2026 22:37

Qard requested changes May 30, 2026

View reviewed changes

Comment thread lib/diagnostics_channel.js Outdated

Comment thread src/node_errors.cc

Comment thread test/parallel/test-diagnostics-channel-suppression.js Outdated

Comment thread test/parallel/test-diagnostics-channel-suppression.js Outdated

DivyanshuX9 added 3 commits May 30, 2026 13:55

lib: rename diagnostics_channel suppression option to subscriberId

a5c4940

Signed-off-by: Divyanshu Sharma <divyanshu88999@gmail.com>

test: clean up diagnostics_channel suppression coverage

7e415c0

Signed-off-by: Divyanshu Sharma <divyanshu88999@gmail.com>

test: simplify diagnostics_channel suppression handler

8e6d7cb

Signed-off-by: Divyanshu Sharma <divyanshu88999@gmail.com>

Qard reviewed May 30, 2026

View reviewed changes

Comment thread lib/diagnostics_channel.js Outdated

Comment thread test/parallel/test-diagnostics-channel-suppression.js Outdated

DivyanshuX9 added 2 commits May 30, 2026 14:54

test: finalize diagnostics_channel suppression test

07e7e97

Signed-off-by: Divyanshu Sharma <divyanshu88999@gmail.com>

diag(suppression-als): use ALS.run bound fn for suppression context; …

9468795

…update tests to use common.mustCall

Qard reviewed May 30, 2026

View reviewed changes

DivyanshuX9 force-pushed the diag/suppression-als branch from 0f6e780 to 8fa6fd8 Compare May 30, 2026 12:27

test: wrap suppressed callbacks with common.mustCall

b8194af

Signed-off-by: Divyanshu Sharma <divyanshu88999@gmail.com>

DivyanshuX9 force-pushed the diag/suppression-als branch from 8fa6fd8 to b8194af Compare May 30, 2026 12:38

DivyanshuX9 added 2 commits May 30, 2026 20:27

src: emit V8 warnings synchronously in PerIsolateMessageListener (rem…

66e0cc9

…ove SetImmediate deferral) Signed-off-by: Divyanshu Sharma <divyanshu88999@gmail.com>

src: reintroduce synchronous V8 warning emission (USE wrapper)

01d9cc0

Signed-off-by: Divyanshu Sharma <divyanshu88999@gmail.com>

Flarna reviewed Jul 2, 2026

View reviewed changes

Comment thread lib/diagnostics_channel.js Outdated

Comment thread lib/diagnostics_channel.js

Comment thread lib/diagnostics_channel.js Outdated

Comment thread lib/diagnostics_channel.js

Comment thread lib/diagnostics_channel.js

diagnostics_channel: add bypass subscriber/store support

365ab7b

doc: add diagnostics_channel bypass API documentation

149f073

Signed-off-by: Divyanshu Sharma <divyanshu88999@gmail.com>

Flarna reviewed Jul 7, 2026

View reviewed changes

doc: address Flarna review feedback on bypass documentation

f7f593e

Signed-off-by: Divyanshu Sharma <divyanshu88999@gmail.com>

DivyanshuX9 force-pushed the diag/suppression-als branch from 2ba3a7a to f7f593e Compare July 8, 2026 13:44

Flarna reviewed Jul 12, 2026

View reviewed changes

Comment thread doc/api/diagnostics_channel.md Outdated

doc: revert unrelated tracePromise description changes

58785bb

Signed-off-by: Divyanshu Sharma <divyanshu88999@gmail.com>

DivyanshuX9 force-pushed the diag/suppression-als branch from 2fccc37 to 58785bb Compare July 13, 2026 13:31

Flarna approved these changes Jul 13, 2026

View reviewed changes

lib: resolve merge conflicts in diagnostics_channel bypass feature

aa285c8

Signed-off-by: DivyanshuX9 <divyanshu88999@gmail.com>

DivyanshuX9 force-pushed the diag/suppression-als branch from 81685dd to aa285c8 Compare July 24, 2026 16:24

DivyanshuX9 and others added 7 commits July 24, 2026 22:00

Merge branch 'main' into diag/suppression-als

f20d4d9

lib: fix merge conflict residue in diagnostics_channel.js

8dd3c97

Signed-off-by: Divyanshu Sharma <divyanshu88999@gmail.com>

lib: fix diagnostics_channel native subscriber count to include bypas…

67b9333

…s entries Signed-off-by: Divyanshu Sharma <divyanshu88999@gmail.com>

Merge branch 'nodejs:main' into diag/suppression-als

fcf72cf

DivyanshuX9 mentioned this pull request Jul 26, 2026

repl: Runtime.awaitPromise races with GC when objectGroup is not passed Body #64762

Closed

Merge branch 'nodejs:main' into diag/suppression-als

a86df32

Uh oh!

Uh oh!

Conversation

DivyanshuX9 commented May 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

What Changed

lib/diagnostics_channel.js

test/parallel/test-diagnostics-channel-suppression.js

Why

Backward Compatibility

Naming

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

DivyanshuX9 commented May 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented May 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Qard left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DivyanshuX9 commented May 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Qard commented May 30, 2026

Uh oh!

DivyanshuX9 commented Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Flarna left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DivyanshuX9 commented Jul 2, 2026

Uh oh!

Flarna commented Jul 3, 2026

Uh oh!

DivyanshuX9 commented Jul 3, 2026

Uh oh!

DivyanshuX9 commented Jul 6, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DivyanshuX9 commented Jul 8, 2026

Uh oh!

DivyanshuX9 commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Flarna commented Jul 8, 2026

Uh oh!

Uh oh!

DivyanshuX9 commented May 29, 2026 •

edited

Loading

`lib/diagnostics_channel.js`

`test/parallel/test-diagnostics-channel-suppression.js`

DivyanshuX9 commented May 29, 2026 •

edited

Loading

codecov Bot commented May 30, 2026 •

edited

Loading

DivyanshuX9 commented May 30, 2026 •

edited

Loading

DivyanshuX9 commented Jul 1, 2026 •

edited

Loading

DivyanshuX9 commented Jul 8, 2026 •

edited

Loading